feat(backend, sdk): Add custom_path field to RuntimeArtifact #12248
Conversation
|
Skipping CI for Draft Pull Request. |
google-oss-prow
bot
requested review from
DharmitD,
droctothorpe,
rimolive and
zazulam
alyssacgoins
force-pushed
the
model-custom-upload-path
branch
9 times, most recently
from
daab84c to
2120dcd
Compare
alyssacgoins
force-pushed
the
model-custom-upload-path
branch
from
2120dcd to
545a55f
Compare
alyssacgoins
force-pushed
the
model-custom-upload-path
branch
from
7934fc1 to
e24f5b1
Compare
alyssacgoins
force-pushed
the
model-custom-upload-path
branch
3 times, most recently
from
29c50a9 to
23b7804
Compare
alyssacgoins
force-pushed
the
model-custom-upload-path
branch
3 times, most recently
from
329d06d to
4d5465a
Compare
alyssacgoins
changed the title
alyssacgoins
force-pushed
the
model-custom-upload-path
branch
from
4d5465a to
a174e85
Compare
| if outputArtifact.CustomPath != nil { | ||
| localDir = *outputArtifact.CustomPath | ||
| } |
There was a problem hiding this comment.
Am I missing something, or does this "duplicate" the above statement?
It seems like it can be removed.
There was a problem hiding this comment.
You're right - I refactored the retrieveArtifactPath() logic after implementing this code block, so I completely missed it. Updated now
backend/src/v2/driver/driver.go
Outdated
| func localPathForURI(uri string) (string, error) { | ||
| if strings.HasPrefix(uri, "gs://") { | ||
| return "/gcs/" + strings.TrimPrefix(uri, "gs://"), nil | ||
| } | ||
| if strings.HasPrefix(uri, "minio://") { | ||
| return "/minio/" + strings.TrimPrefix(uri, "minio://"), nil | ||
| } | ||
| if strings.HasPrefix(uri, "s3://") { | ||
| return "/s3/" + strings.TrimPrefix(uri, "s3://"), nil | ||
| } | ||
| if strings.HasPrefix(uri, "oci://") { | ||
| return "/oci/" + strings.ReplaceAll(strings.TrimPrefix(uri, "oci://"), "/", "_") + "/models", nil | ||
| } | ||
| return "", fmt.Errorf("failed to generate local path for URI %s: unsupported storage scheme", uri) | ||
| } |
There was a problem hiding this comment.
This duplicates the logic from backend/src/v2/component/launcher_v2.go.
Can we keep the component.LocalPathForURI function and use that in retrieveArtifactPath (in else)? With that, we can revert all the changes in this file.
alyssacgoins
force-pushed
the
model-custom-upload-path
branch
2 times, most recently
from
be47260 to
815bbf2
Compare
|
@alyssacgoins can you please rebase? |
alyssacgoins
force-pushed
the
model-custom-upload-path
branch
from
815bbf2 to
d7399d7
Compare
droctothorpe
force-pushed
the
model-custom-upload-path
branch
from
d7399d7 to
b0371c0
Compare
|
/retest |
|
/lgtm |
alyssacgoins
force-pushed
the
model-custom-upload-path
branch
from
30bb62d to
d7399d7
Compare
droctothorpe
force-pushed
the
model-custom-upload-path
branch
from
d7399d7 to
5551af8
Compare
Signed-off-by: agoins <[email protected]>
Signed-off-by: agoins <[email protected]>
alyssacgoins
force-pushed
the
model-custom-upload-path
branch
from
5551af8 to
fe7b8f9
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: droctothorpe The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Resolves #12386
Description of your changes:
Add a
custom_pathfield to theRuntimeArtifactobject that overrides the object's URI field during artifact upload, allowing artifacts to be uploaded from non-local storage.Changes validated via unit test in
launcher_v2.Test_retrieve_artifact_path()as well as the sample pipelinecomponent_with_runtime_artifact_custom_path.pyadded totest_data/sdk_compiled_pipelines/valid/critical, which sets and validates a custom artifact path.Checklist: